Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connectivity: introduce host firewall tests #2464

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

viktor-kurchenko
Copy link
Contributor

@viktor-kurchenko viktor-kurchenko commented Apr 2, 2024

Introduce two new tests covering the host firewall functionality, i.e., asserting that both ingress and egress CiliumClusterwideNetworkPolicies specifying a NodeSelector correctly block the expected traffic. The tests are executed only when the unsafe tests are enabled, as potentially disruptive if executed against a live cluster.

Original commit by Marco Iorio
7a928d6
has been accidentally dropped by the following commit during merge
486eb99

@viktor-kurchenko
Copy link
Contributor Author

@giorio94 I'm really sorry but I've lost these two tests after resolving conflicts for this PR: #2322.

How should we proceed with it? Are you ok if it will be merged under my account?

@giorio94
Copy link
Member

giorio94 commented Apr 4, 2024

@giorio94 I'm really sorry but I've lost these two tests after resolving conflicts for this PR: #2322.

How should we proceed with it? Are you ok if it will be merged under my account?

No problem. Could you please just extend the commit message to link to the commit that originally introduced the tests and the one that dropped them, for the sake of traceability? Please also note that you'll also need to initialize the ccnps map here, which apparently also got lost in the rebase.

@viktor-kurchenko viktor-kurchenko force-pushed the pr/vk/host-firewall-tests branch from bcc9a07 to e6d2fce Compare April 4, 2024 12:20
@viktor-kurchenko viktor-kurchenko marked this pull request as ready for review April 4, 2024 12:20
@viktor-kurchenko viktor-kurchenko requested a review from a team as a code owner April 4, 2024 12:20
@viktor-kurchenko
Copy link
Contributor Author

@giorio94 I'm really sorry but I've lost these two tests after resolving conflicts for this PR: #2322.
How should we proceed with it? Are you ok if it will be merged under my account?

No problem. Could you please just extend the commit message to link to the commit that originally introduced the tests and the one that dropped them, for the sake of traceability? Please also note that you'll also need to initialize the ccnps map here, which apparently also got lost in the rebase.

Done, sorry again!

Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@michi-covalent
Copy link
Contributor

hello @viktor-kurchenko , we are in the process of moving the connectivity package from cilium-cli to cilium repo. there are 2 pending pull requests:

once these 2 pull requests are merged, you need to open a pull request in cilium/cilium repository instead. sorry for the inconvenience!

a cilium slack thread in #testing channel: https://cilium.slack.com/archives/C7PE7V806/p1711552887006539

@viktor-kurchenko
Copy link
Contributor Author

hello @viktor-kurchenko , we are in the process of moving the connectivity package from cilium-cli to cilium repo. there are 2 pending pull requests:

once these 2 pull requests are merged, you need to open a pull request in cilium/cilium repository instead. sorry for the inconvenience!

a cilium slack thread in #testing channel: https://cilium.slack.com/archives/C7PE7V806/p1711552887006539

Gotcha! Will do, thanks!

@michi-covalent
Copy link
Contributor

the connectivity package came back to cilium-cli repo: #2477

you can re-open this pull request 🚀🙏

@michi-covalent
Copy link
Contributor

could you rebase to get rid of Don't modify the connectivity command workflow? thanks!

@michi-covalent michi-covalent added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 11, 2024
Introduce two new tests covering the host firewall functionality, i.e.,
asserting that both ingress and egress CiliumClusterwideNetworkPolicies
specifying a NodeSelector correctly block the expected traffic. The
tests are executed only when the unsafe tests are enabled, as
potentially disruptive if executed against a live cluster.

Original commit by Marco Iorio
7a928d6
has been accidentally dropped by the following commit during merge
486eb99

Signed-off-by: viktor-kurchenko <[email protected]>
@viktor-kurchenko
Copy link
Contributor Author

could you rebase to get rid of Don't modify the connectivity command workflow? thanks!

Done, thanks!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 11, 2024
@michi-covalent michi-covalent merged commit c1ce402 into main Apr 11, 2024
12 of 13 checks passed
@michi-covalent michi-covalent deleted the pr/vk/host-firewall-tests branch April 11, 2024 20:36
@brb
Copy link
Member

brb commented Apr 12, 2024

Sweet! Maybe we can get rid of https://github.com/cilium/cilium/blob/main/test/k8s/datapath_configuration.go#L574 once the CLI test is being in use by cilium/cilium?

@giorio94
Copy link
Member

Sweet! Maybe we can get rid of https://github.com/cilium/cilium/blob/main/test/k8s/datapath_configuration.go#L574 once the CLI test is being in use by cilium/cilium?

AFAICT, the main missing part would be the host-to-host coverage, which seems to be included in the Ginkgo tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase This PR needs to be rebased because it has merge conflicts. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants